Skip to content

fix(state): operation argument in provide/process#4712

Merged
soyuka merged 6 commits into
api-platform:mainfrom
soyuka:fix/state-interfaces
Apr 19, 2022
Merged

fix(state): operation argument in provide/process#4712
soyuka merged 6 commits into
api-platform:mainfrom
soyuka:fix/state-interfaces

Conversation

@soyuka

@soyuka soyuka commented Apr 12, 2022

Copy link
Copy Markdown
Member

Review the state provider and processor interfaces. Move provider/processor on the Operation.

use Doctrine\ORM\EntityManagerInterface;
use Doctrine\Persistence\ManagerRegistry;

class DoctrineOrmResourceCollectionMetadataFactory implements ResourceMetadataCollectionFactoryInterface

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class DoctrineOrmResourceCollectionMetadataFactory implements ResourceMetadataCollectionFactoryInterface
final class DoctrineOrmResourceCollectionMetadataFactory implements ResourceMetadataCollectionFactoryInterface


class DoctrineOrmResourceCollectionMetadataFactory implements ResourceMetadataCollectionFactoryInterface
{
private $decorated;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing types?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop php 7.1 and yes if not we can't or as phpdoc

private function getProvider(AbstractOperation $operation): string
{
if ($operation instanceof CollectionOperationInterface) {
return 'api_platform.doctrine.orm.state.collection_provider';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should alias these services with the FQCN, and use it here. This will remove the hard coupling with Symfony.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried just aliasing and it doesn't work with tags + service locators this was a quick fix I need to use the class as id

Comment thread src/Metadata/AbstractOperation.php Outdated

namespace ApiPlatform\Metadata;

abstract class AbstractOperation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
abstract class AbstractOperation
abstract class Operation

The API will be nicer, and this is consistent with the newest coding convention used in Symfony.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know right, but we need to rename the current one then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue as this was never released isn't it?

Comment thread src/State/CallableProcessor.php Outdated
use ApiPlatform\Metadata\AbstractOperation;
use Psr\Container\ContainerInterface;

class CallableProcessor implements ProcessorInterface

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class CallableProcessor implements ProcessorInterface
final class CallableProcessor implements ProcessorInterface

Comment thread src/State/LegacyDataProviderState.php Outdated
public function provide(AbstractOperation $operation, array $uriVariables = [], array $context = [])
{
$operation = $context['operation'] ?? null;
exit('hello');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔨

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on purpose I need to fix this

use ApiPlatform\Metadata\AbstractOperation;
use ApiPlatform\State\ProcessorInterface;

class ServiceLocatorProcessor implements ProcessorInterface

@dunglas dunglas Apr 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ServiceLocatorProcessor implements ProcessorInterface
final class ServiceLocatorProcessor implements ProcessorInterface

And as this class doesn't depend on Symfony, it could be placed in the top-level namespace.

@soyuka
soyuka force-pushed the fix/state-interfaces branch 3 times, most recently from d7a9eb2 to ebb6151 Compare April 19, 2022 10:18
@soyuka
soyuka force-pushed the fix/state-interfaces branch from 372bd07 to 6ccbaa3 Compare April 19, 2022 14:05
@soyuka
soyuka marked this pull request as ready for review April 19, 2022 14:14
@soyuka
soyuka merged commit ee0fabb into api-platform:main Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants